[grid][java]: session-timeout set connection timeout in RemoteNode #13854

VietND96 · 2024-04-22T16:08:39Z

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Try something for #13340
session-timeout is an attribute of all node types and can get in part of NodeStatus
Use session-timeout to set connection timeout and read timeout for HttpClient config in RemoteNode

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I have read the contributing document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

Type

enhancement

Description

Added sessionTimeout to NodeStatus and updated JSON handling.
Enhanced various node types and distributor classes to include and manage sessionTimeout.
Updated tests across multiple classes to incorporate sessionTimeout in node configurations and assertions.

Changes walkthrough

Relevant files

Enhancement

9 files

NodeStatus.java `Add session timeout to NodeStatus` java/src/org/openqa/selenium/grid/data/NodeStatus.java Added `sessionTimeout` field to `NodeStatus` class. Updated constructors and JSON serialization/deserialization to handle `sessionTimeout`.	+22/-1
AddNode.java `Include session timeout in node creation` java/src/org/openqa/selenium/grid/distributor/AddNode.java Modified `execute` method to include `sessionTimeout` when creating a new node.	+1/-0
GridModel.java `Handle session timeout in GridModel` java/src/org/openqa/selenium/grid/distributor/GridModel.java Updated `rewrite` and `amend` methods to handle `sessionTimeout`.	+2/-0
LocalDistributor.java `Adjust node registration to include session timeout` java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java Adjusted `register` method to include `sessionTimeout` in node creation.	+1/-0
Node.java `Add session timeout management in Node base class` java/src/org/openqa/selenium/grid/node/Node.java Added `sessionTimeout` to the Node class and updated constructors. Provided a getter for `sessionTimeout`.	+12/-0
OneShotNode.java `Manage session timeout in OneShotNode` java/src/org/openqa/selenium/grid/node/k8s/OneShotNode.java Included `sessionTimeout` in the constructor and during node creation.	+4/-1
LocalNode.java `Update LocalNode to handle session timeout` java/src/org/openqa/selenium/grid/node/local/LocalNode.java Updated constructors to handle `sessionTimeout`.	+7/-1
RemoteNode.java `Enhance RemoteNode with session timeout configuration` java/src/org/openqa/selenium/grid/node/remote/RemoteNode.java Updated constructors to handle `sessionTimeout`. Configured HTTP client to use `sessionTimeout` for read timeout.	+8/-3
GridRedisClient.java `Include session timeout in Redis client node retrieval` java/src/org/openqa/selenium/redis/GridRedisClient.java Updated `getNode` method to include `sessionTimeout` in the node status.	+1/-0

Tests

4 files

NodeStatusTest.java `Update NodeStatus tests to include session timeout` java/test/org/openqa/selenium/grid/data/NodeStatusTest.java Updated test to include `sessionTimeout` in `NodeStatus`.	+1/-0
AddingNodesTest.java `Update distributor tests for session timeout handling` java/test/org/openqa/selenium/grid/distributor/AddingNodesTest.java Updated tests to handle `sessionTimeout` in node status events and status retrieval.	+2/-0
DefaultSlotSelectorTest.java `Update slot selector tests to include session timeout` java/test/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelectorTest.java Adjusted node creation in tests to include `sessionTimeout`.	+1/-0
NodeTest.java `Update Node tests to handle session timeout` java/test/org/openqa/selenium/grid/node/NodeTest.java Updated node creation in tests to handle `sessionTimeout`.	+4/-0

✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>

VietND96 · 2024-04-22T16:09:39Z

@diemol, can you also review this one?

qodo-merge-pro · 2024-04-22T16:10:01Z

PR Description updated to latest commit (63872ee)

Copy walkthrough table to "Files Changed" Tab

qodo-merge-pro · 2024-04-22T16:10:21Z

PR Review

(Review updated until commit `2eac37a`)

⏱️ Estimated effort to review [1-5]	3, because the PR involves multiple changes across various files, including core classes like Node, RemoteNode, and NodeStatus. The changes are not overly complex but require a thorough understanding of the existing architecture and the implications of adding session timeout settings.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: Ensure that all constructors and methods that now include `sessionTimeout` properly handle this new parameter. There is a risk of null values or incorrect timeout settings being propagated if not handled correctly.
🔍 Possible issues	Performance Concern: Adding session timeout to the HTTP client configuration could potentially impact performance. It's important to test how these changes affect the system's responsiveness and timeout behavior under different network conditions.
🔒 Security concerns	No

✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

qodo-merge-pro · 2024-04-22T16:10:59Z

PR Code Suggestions

Category	Suggestions
Enhancement	Add validation to ensure the session timeout is not negative or zero. The constructor `Node` should validate the `sessionTimeout` parameter to ensure it is not negative or zero. This will prevent potential runtime issues related to invalid timeout values. java/src/org/openqa/selenium/grid/node/Node.java [133] -this.sessionTimeout = Require.nonNull("Session timeout", sessionTimeout); +this.sessionTimeout = Require.nonNegative("Session timeout", sessionTimeout);
Performance	Modify the `ClientConfig` to have different values for connection and read timeouts. When setting the `ClientConfig` for the `RemoteNode`, it is important to ensure that the read and connection timeouts are set appropriately to handle network variability. Consider adding a buffer or using different values for connection and read timeouts. java/src/org/openqa/selenium/grid/node/remote/RemoteNode.java [95-99] ClientConfig clientConfig = defaultConfig() .connectionTimeout(this.getSessionTimeout()) - .readTimeout(this.getSessionTimeout()) + .readTimeout(this.getSessionTimeout().plusSeconds(30)) .baseUrl(fromUri(externalUri));
Maintainability	Add logging for the session timeout value in the `Node` class. In the `Node` class, when setting the `sessionTimeout`, it's beneficial to log this value for debugging purposes, especially when issues related to session management occur. java/src/org/openqa/selenium/grid/node/Node.java [133] this.sessionTimeout = Require.nonNull("Session timeout", sessionTimeout); +System.out.println("Session timeout set to: " + sessionTimeout);
Bug	Ensure the session timeout is non-null before passing to the superclass constructor in `OneShotNode`. Ensure that the `sessionTimeout` parameter is passed correctly to the superclass constructor in the `OneShotNode` class to maintain consistency and avoid potential bugs. java/src/org/openqa/selenium/grid/node/k8s/OneShotNode.java [118] -super(tracer, id, uri, registrationSecret, sessionTimeout); +super(tracer, id, uri, registrationSecret, Require.nonNull("Session timeout", sessionTimeout));
Best practice	Validate the `sessionTimeout` parameter in the `LocalNode` constructor. In the `LocalNode` constructor, ensure that the `sessionTimeout` is passed correctly and validated similarly to other parameters to prevent inconsistencies. java/src/org/openqa/selenium/grid/node/local/LocalNode.java [157] -super(tracer, new NodeId(UUID.randomUUID()), uri, registrationSecret, sessionTimeout); +super(tracer, new NodeId(UUID.randomUUID()), uri, registrationSecret, Require.nonNull("Session timeout", sessionTimeout));

✨ Improve tool usage guide:

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>

qodo-merge-pro · 2024-04-26T05:00:18Z

PR Description updated to latest commit (2eac37a)

Copy walkthrough table to "Files Changed" Tab

qodo-merge-pro · 2024-04-26T05:00:29Z

Persistent review updated to latest commit 2eac37a

qodo-merge-pro · 2024-04-26T05:01:13Z

PR Code Suggestions

Category	Suggestions
Bug	Add null check for `sessionTimeout` to ensure it is not null and positive. Ensure that the `sessionTimeout` parameter is validated for null values to prevent potential `NullPointerExceptions` when the timeout is used. java/src/org/openqa/selenium/grid/node/Node.java [133] -this.sessionTimeout = Require.positive("Session timeout", sessionTimeout); +this.sessionTimeout = Require.nonNullAndPositive("Session timeout", sessionTimeout);
Enhancement	Add a fallback for `sessionTimeout` in `ClientConfig` to handle potential null values. Modify the `ClientConfig` to include a fallback timeout value in case `sessionTimeout` is not set, ensuring robustness. java/src/org/openqa/selenium/grid/node/remote/RemoteNode.java [95-96] ClientConfig clientConfig = - defaultConfig().readTimeout(this.getSessionTimeout()).baseUrl(fromUri(externalUri)); + defaultConfig().readTimeout(Optional.ofNullable(this.getSessionTimeout()).orElse(Duration.ofSeconds(30))).baseUrl(fromUri(externalUri));
Best practice	Ensure `sessionTimeout` is within a reasonable range to avoid resource hogging. Add a check to ensure that `sessionTimeout` is not excessively long, which could cause resources to be held unnecessarily. java/src/org/openqa/selenium/grid/node/k8s/OneShotNode.java [118] -super(tracer, id, uri, registrationSecret, Require.positive(sessionTimeout)); +super(tracer, id, uri, registrationSecret, Require.positiveAndNotExcessive("Session timeout", sessionTimeout, Duration.ofHours(1)));
Maintainability	Log a warning if `sessionTimeout` is set to a value that might negatively impact performance. Since `sessionTimeout` is crucial for node status, consider logging a warning if it is set to a very low value which might affect node performance. java/src/org/openqa/selenium/grid/data/NodeStatus.java [67] this.sessionTimeout = sessionTimeout; +if (sessionTimeout.compareTo(Duration.ofMinutes(1)) < 0) { + Logger.warn("Session timeout is set very low. It might affect node performance."); +}
Maintainability	Refactor `sessionTimeout` handling in the constructor for clarity and maintainability. Refactor the constructor to handle `sessionTimeout` more explicitly by separating its validation from the super constructor call for clarity. java/src/org/openqa/selenium/grid/node/local/LocalNode.java [157-162] -super( - tracer, - new NodeId(UUID.randomUUID()), - uri, - registrationSecret, - Require.positive(sessionTimeout)); +Duration validatedTimeout = Require.positive("Session timeout", sessionTimeout); +super(tracer, new NodeId(UUID.randomUUID()), uri, registrationSecret, validatedTimeout);

✨ Improve tool usage guide:

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

diemol · 2024-05-06T08:24:41Z

@VietND96 can you please update your fork?

java/src/org/openqa/selenium/grid/node/Node.java

java/src/org/openqa/selenium/grid/node/remote/RemoteNode.java

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>

diemol

Need to update the fork.

VietND96 · 2024-05-06T12:06:29Z

@diemol, I updated the fork. Also, I found that the test is used to define a CustomNode, and it can be added. So, I updated the CustomNode with attr sessionTimeout, which can be set when creating. In the test, it asserts that getSessionTimeout() is working. Check if the update for the test makes sense.

codecov · 2024-05-06T12:26:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.70%. Comparing base (fe2edbd) to head (3548225).
Report is 2 commits behind head on trunk.

❗ Current head 3548225 differs from pull request most recent head e199835. Consider uploading reports for the commit e199835 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #13854   +/-   ##
=======================================
  Coverage   58.70%   58.70%           
=======================================
  Files          86       86           
  Lines        5296     5296           
  Branches      226      226           
=======================================
  Hits         3109     3109           
  Misses       1961     1961           
  Partials      226      226

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…eleniumHQ#13854) * [grid][java]: add session-timeout to all kind of Nodes Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com> * [grid][java]: fix code as suggestions Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com> * [java][grid]: update test CustomNode can get/set sessionTimeout Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com> --------- Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>

VietND96 added 2 commits April 22, 2024 14:55

[grid][java]: add session-timeout to all kind of Nodes

63872ee

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>

Merge branch 'trunk' into node-timeout

79a9449

qodo-merge-pro bot added the enhancement label Apr 22, 2024

qodo-merge-pro bot added the Review effort [1-5]: 3 label Apr 22, 2024

[grid][java]: fix code as suggestions

e66bc47

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>

diemol added this to the 4.21 milestone Apr 23, 2024

VietND96 added 2 commits April 23, 2024 13:57

Merge branch 'trunk' into node-timeout

2c9ff73

Merge branch 'trunk' into node-timeout

2eac37a

VietND96 closed this Apr 26, 2024

VietND96 deleted the node-timeout branch April 26, 2024 04:58

VietND96 restored the node-timeout branch April 26, 2024 04:58

VietND96 reopened this Apr 26, 2024

VietND96 added 3 commits May 2, 2024 07:14

Merge branch 'trunk' into node-timeout

1a2791e

Merge branch 'trunk' into node-timeout

4ed2e9b

Merge branch 'trunk' into node-timeout

c41b7b1

Merge branch 'trunk' into node-timeout

bc16a30

diemol linked an issue May 6, 2024 that may be closed by this pull request

[🚀 Feature]: The ability to change the client timeout of Selenium Grid to more than 3 minutes #13340

Closed

diemol reviewed May 6, 2024

View reviewed changes

java/src/org/openqa/selenium/grid/node/Node.java Outdated Show resolved Hide resolved

VietND96 commented May 6, 2024

View reviewed changes

java/src/org/openqa/selenium/grid/node/remote/RemoteNode.java Show resolved Hide resolved

[java][grid]: update test CustomNode can get/set sessionTimeout

3548225

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>

diemol approved these changes May 6, 2024

View reviewed changes

VietND96 force-pushed the node-timeout branch from c4db212 to 3548225 Compare May 6, 2024 12:00

Merge branch 'trunk' into node-timeout

e199835

diemol merged commit 72562d8 into SeleniumHQ:trunk May 6, 2024
34 of 36 checks passed

VietND96 deleted the node-timeout branch May 6, 2024 13:44

MiguelPelegrina mentioned this pull request Jun 6, 2024

[Snyk] Upgrade selenium-webdriver from 4.20.0 to 4.21.0 MiguelPelegrina/java_school_online_store_frontend#16

Merged

x-way mentioned this pull request Jun 10, 2024

[Snyk] Upgrade selenium-webdriver from 4.18.1 to 4.21.0 x-way/cal-heatmap#451

Closed

farmmanic mentioned this pull request Jun 14, 2024

[Snyk] Upgrade selenium-webdriver from 4.3.0 to 4.21.0 farmmanic/social-analyzer#15

Open

LunarArtemis mentioned this pull request Jun 22, 2024

[Snyk] Upgrade selenium-webdriver from 4.19.0 to 4.21.0 LunarArtemis/tuktuk#4

Open

MiguelPelegrina mentioned this pull request Jul 12, 2024

[Snyk] Upgrade selenium-webdriver from 4.21.0 to 4.22.0 MiguelPelegrina/java_school_online_store_frontend#19

Merged

aravindb26 mentioned this pull request Jul 16, 2024

[Snyk] Upgrade selenium-webdriver from 4.16.0 to 4.22.0 aravindb26/spark#3

Open

n1ghtmar3-6251 mentioned this pull request Jul 17, 2024

[Snyk] Upgrade selenium-webdriver from 4.11.1 to 4.22.0 n1ghtmar3-6251/spark#3

Open

x-way mentioned this pull request Jul 24, 2024

[Snyk] Upgrade selenium-webdriver from 4.18.1 to 4.22.0 x-way/cal-heatmap#503

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[grid][java]: session-timeout set connection timeout in RemoteNode #13854

[grid][java]: session-timeout set connection timeout in RemoteNode #13854

VietND96 commented Apr 22, 2024 •

edited by qodo-merge-pro bot

Loading

VietND96 commented Apr 22, 2024

qodo-merge-pro bot commented Apr 22, 2024

qodo-merge-pro bot commented Apr 22, 2024 •

edited

Loading

qodo-merge-pro bot commented Apr 22, 2024

qodo-merge-pro bot commented Apr 26, 2024

qodo-merge-pro bot commented Apr 26, 2024

qodo-merge-pro bot commented Apr 26, 2024

diemol commented May 6, 2024

diemol left a comment

VietND96 commented May 6, 2024

codecov bot commented May 6, 2024

[grid][java]: session-timeout set connection timeout in RemoteNode #13854

[grid][java]: session-timeout set connection timeout in RemoteNode #13854

Conversation

VietND96 commented Apr 22, 2024 • edited by qodo-merge-pro bot Loading

User description

Description

Motivation and Context

Types of changes

Checklist

Type

Description

Changes walkthrough

VietND96 commented Apr 22, 2024

qodo-merge-pro bot commented Apr 22, 2024

qodo-merge-pro bot commented Apr 22, 2024 • edited Loading

PR Review

(Review updated until commit 2eac37a)

qodo-merge-pro bot commented Apr 22, 2024

PR Code Suggestions

qodo-merge-pro bot commented Apr 26, 2024

qodo-merge-pro bot commented Apr 26, 2024

qodo-merge-pro bot commented Apr 26, 2024

PR Code Suggestions

diemol commented May 6, 2024

diemol left a comment

Choose a reason for hiding this comment

VietND96 commented May 6, 2024

codecov bot commented May 6, 2024

Codecov Report

VietND96 commented Apr 22, 2024 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Apr 22, 2024 •

edited

Loading

(Review updated until commit `2eac37a`)